feat(log): remove core dependency and update core interface to be dependency free #21045
Conversation
|
Important Review skippedMore than 25% of the files skipped due to max files limit. The review is being skipped to prevent a low-quality review. 20 files out of 77 files are above the max files limit of 50. Please upgrade to Pro plan to get higher limits. You can disable this status message by setting the WalkthroughWalkthroughThe changes enhance the logging system in the Cosmos SDK by restructuring the logging interfaces, introducing Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant LoggerV2
participant LoggerBase
Client->>LoggerV2: Create Logger
LoggerV2->>LoggerBase: Initialize with base methods
LoggerV2->>LoggerV2: Add context with WithContext()
LoggerV2->>LoggerBase: Log Info/Debug messages
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
core/log/logger.go
Outdated
| type LoggerV2 interface { | ||
| LoggerBase | ||
|
|
||
| // WithV2 returns a type which can be cast to LoggerV2 with additional context provided by a set. | ||
| // any is returned instead of LoggerV2 to avoid the need for log users to import the log package directly. | ||
| WithV2(keyVals ...any) any | ||
| } |
There was a problem hiding this comment.
I'm not attached to this naming if someone can think of something better.
There was a problem hiding this comment.
AH! I was scratching my head yesterday about how to achieve the same thing main...julien/log-dep :D
We quite need log to have no core dep as well actually.
There was a problem hiding this comment.
I actually don't like WithV2 in a function name if this is going to be long lived. How about WithContext?
This comment has been minimized.
This comment has been minimized.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (3)
log/testing.go (2)
28-30: Update the type ofrunLoggertoLoggerV2The variable
runLoggerintestutils/sims/runner.gois currently declared aslog.Logger. SinceNewTestLoggerInfonow returnsLoggerV2, you should update the type ofrunLoggertoLoggerV2to ensure type consistency.
testutils/sims/runner.go: Update the declaration ofrunLoggertoLoggerV2.Analysis chain
LGTM! But verify the function usage in the codebase.
The change to return
LoggerV2instead ofLoggeris approved.However, ensure that all function calls to
NewTestLoggerInfoare correctly handled with the new return type.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `NewTestLoggerInfo` match the new return type. # Test: Search for the function usage. Expect: Only occurrences of the new return type. rg --type go -A 5 $'NewTestLoggerInfo'Length of output: 1240
Script:
#!/bin/bash # Description: Verify all usages of `runLogger` in `testutils/sims/runner.go` to ensure it is handled as `LoggerV2`. # Test: Search for all occurrences of `runLogger` in the file. rg --type go 'runLogger' testutils/sims/runner.goLength of output: 253
37-39: Type mismatch detected forloggerintestutils/sims/runner.go.The variable
loggeris currently of typelog.Logger, butNewTestLoggerErrornow returnsLoggerV2. This needs to be corrected to avoid type incompatibility issues.
- Update the type of
loggerintestutils/sims/runner.gotoLoggerV2.Analysis chain
LGTM! But verify the function usage in the codebase.
The change to return
LoggerV2instead ofLoggeris approved.However, ensure that all function calls to
NewTestLoggerErrorare correctly handled with the new return type.
Ensure
loggerintestutils/sims/runner.gois of typeLoggerV2.The function
NewTestLoggerErroris used intestutils/sims/runner.go. We need to confirm that the variableloggeris of the correct type (LoggerV2).Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `NewTestLoggerError` match the new return type. # Test: Search for the function usage. Expect: Only occurrences of the new return type. rg --type go -A 5 $'NewTestLoggerError'Length of output: 1266
Script:
#!/bin/bash # Description: Verify the type of the variable `logger` in `testutils/sims/runner.go`. # Test: Search for the variable declaration and type. Expect: The type should be `LoggerV2`. rg --type go 'var logger' -A 5 testutils/sims/runner.goLength of output: 191
log/logger.go (1)
Update variable types to match the new return type
LoggerV2The function
NewLoggernow returnsLoggerV2instead ofLogger. However, there are multiple instances in the codebase where the returned value is assigned to a variable of typelog.Logger. This discrepancy needs to be addressed to ensure type compatibility.Please update the variable types to match the new return type
LoggerV2in the following files:
testutil/sims/simulation_helpers.gotestutil/network/network.gotools/cosmovisor/args.gotools/cosmovisor/upgrade_test.gotools/cosmovisor/process_test.gotools/cosmovisor/cmd/cosmovisor/init_test.gotests/integration/example/example_test.goserver/cmt_cmds.goserver/v2/server_test.goserver/v2/store/commands.goserver/v2/logger.goserver/v2/commands.goserver/util.gosimapp/v2/simdv2/cmd/commands.golog/logger.golog/writer_test.golog/logger_test.goclient/snapshot/restore.goclient/snapshot/export.goclient/pruning/main.goclient/cmd.gobaseapp/baseapp_test.goAnalysis chain
Line range hint
77-103:
LGTM! But verify the function usage in the codebase.The change to return
LoggerV2instead ofLoggeris approved.However, ensure that all function calls to
NewLoggerare correctly handled with the new return type.
Let's verify if the instances of
NewLoggerare correctly handling the new return typeLoggerV2.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `NewLogger` match the new return type. # Test: Search for the function usage. Expect: Only occurrences of the new return type. rg --type go -A 5 $'NewLogger'Length of output: 10612
Script:
#!/bin/bash # Description: Verify if the returned value from `NewLogger` is used correctly as `LoggerV2`. # Test: Check the type usage of the returned value from `NewLogger` in the identified files. # List of files to check files=( "testutil/sims/simulation_helpers.go" "testutil/network/network.go" "tools/cosmovisor/args.go" "tools/cosmovisor/upgrade_test.go" "tools/cosmovisor/process_test.go" "tools/cosmovisor/cmd/cosmovisor/init_test.go" "tests/integration/example/example_test.go" "server/cmt_cmds.go" "server/v2/server_test.go" "server/v2/store/commands.go" "server/v2/logger.go" "server/v2/commands.go" "server/util.go" "simapp/v2/simdv2/cmd/commands.go" "log/logger.go" "log/writer_test.go" "log/logger_test.go" "client/snapshot/restore.go" "client/snapshot/export.go" "client/pruning/main.go" "client/cmd.go" "baseapp/baseapp_test.go" ) # Search for the usage of the returned value from `NewLogger` in each file for file in "${files[@]}"; do echo "Checking file: $file" rg --type go --context 5 'log.NewLogger' "$file" doneLength of output: 13580
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (3)
- core/log/logger.go (2 hunks)
- log/logger.go (5 hunks)
- log/testing.go (3 hunks)
Additional context used
Path-based instructions (3)
log/testing.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.core/log/logger.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.log/logger.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (10)
log/testing.go (2)
19-21: LGTM! But verify the function usage in the codebase.The change to return
LoggerV2instead ofLoggeris approved.However, ensure that all function calls to
NewTestLoggerare correctly handled with the new return type.
Line range hint
41-51:
LGTM! But verify the function usage in the codebase.The change to return
LoggerV2instead ofLoggeris approved.However, ensure that all function calls to
newTestLoggerare correctly handled with the new return type.Verification successful
The change to return
LoggerV2instead ofLoggeris correctly handled.The functions
NewTestLoggerInfoandNewTestLoggerErrorare used appropriately intestutils/sims/runner.go, and theLoggerV2type is correctly managed in these instances.
NewTestLoggerInfois assigned torunLoggerand used with theWithmethod.NewTestLoggerErroris assigned tologgerand used with theWithmethod.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `newTestLogger` match the new return type. # Test: Search for the function usage. Expect: Only occurrences of the new return type. rg --type go -A 5 $'newTestLogger'Length of output: 916
Script:
#!/bin/bash # Description: Verify the usage of functions that call `newTestLogger` to ensure the return type `LoggerV2` is correctly handled. # Test: Search for the usage of functions that call `newTestLogger`. rg --type go -A 5 'NewTestLoggerDebug\(\)' rg --type go -A 5 'NewTestLoggerInfo\(\)' rg --type go -A 5 'NewTestLoggerError\(\)'Length of output: 133
Script:
#!/bin/bash # Description: Verify the usage of functions that call `newTestLogger` to ensure the return type `LoggerV2` is correctly handled. # Test: Search for the usage of functions that call `newTestLogger` with a more general pattern. rg --type go 'NewTestLogger(Debug|Info|Error)'Length of output: 547
Script:
#!/bin/bash # Description: Verify the usage of `NewTestLoggerInfo` and `NewTestLoggerError` in `testutils/sims/runner.go` to ensure the return type `LoggerV2` is correctly handled. # Test: Search for the usage of `NewTestLoggerInfo` and `NewTestLoggerError` in `testutils/sims/runner.go`. rg --type go -A 5 'NewTestLoggerInfo\(' testutils/sims/runner.go rg --type go -A 5 'NewTestLoggerError\(' testutils/sims/runner.goLength of output: 484
core/log/logger.go (3)
10-14: LGTM!The change to embed
LoggerBaseand include theWithmethod in theLoggerinterface is approved.
Line range hint
16-38:
LGTM!The
LoggerBaseinterface defines basic logging functionalities and the change is approved.
40-48: LGTM!The change to embed
LoggerBaseand include theWithV2method in theLoggerV2interface is approved.log/logger.go (5)
43-47: LGTM!The change to combine the existing
Loggerand the newcorelog.LoggerV2interfaces in theLoggerV2type is approved.
148-151: LGTM! But verify the function usage in the codebase.The change to return
corelog.Loggerinstead ofLoggeris approved.However, ensure that all function calls to
Withare correctly handled with the new return type.
153-155: LGTM! But verify the function usage in the codebase.The addition of the
WithV2function is approved.However, ensure that all function calls to
WithV2are correctly handled.Verification successful
LGTM! But verify the function usage in the codebase.
The addition of the
WithV2function is approved. Both definitions inlog/logger.golook correct, and the comment incore/log/logger.gois consistent with the function's purpose.However, ensure that all function calls to
WithV2are correctly handled.
log/logger.go:
func (l zeroLogWrapper) WithV2(keyVals ...interface{}) anyfunc (l LogWrapper) WithV2(keyVals ...interface{}) anyScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `WithV2` match the new function. # Test: Search for the function usage. Expect: Only occurrences of the new function. rg --type go -A 5 $'WithV2'Length of output: 912
173-175: LGTM! But verify the function usage in the codebase.The change to return
LoggerV2instead ofLoggeris approved.However, ensure that all function calls to
NewLogWrapperare correctly handled with the new return type.Verification successful
Verified: All instances of
NewLogWrappercorrectly handle the new return typeLoggerV2.The change to return
LoggerV2instead ofLoggeris correctly implemented and consistent across the codebase.
log/logger.go: All usages ofNewLogWrapperare correctly updated.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `NewLogWrapper` match the new return type. # Test: Search for the function usage. Expect: Only occurrences of the new return type. rg --type go -A 5 $'NewLogWrapper'Length of output: 659
Script:
#!/bin/bash # Description: Verify all function calls to `NewLogWrapper` match the new return type across the entire codebase. # Test: Search for the function usage in the entire codebase. Expect: Only occurrences of the new return type. rg --type go -A 5 'NewLogWrapper'Length of output: 659
119-121: LGTM! But verify the function usage in the codebase.The change to return
LoggerV2instead ofLoggeris approved.However, ensure that all function calls to
NewCustomLoggerare correctly handled with the new return type.
cosmossdk.io/log is published as 1.x: https://pkg.go.dev/cosmossdk.io/log |
|
Ah whoops. Then this makes sense. Alternatively we can do a v2 pkg |
I think it would be nice to support this somehow with v1. So the most minimal version of this PR involves adding a |
Remove the dependency on core? |
There was a problem hiding this comment.
ACK. I'd argue then that the actual corelog.Logger (with With(...) Logger) can be moved back to log.
We can make the SDK use LoggerV2 thorough and kill all deps on core in log (started yesterday here and this is the perfect addition: main...julien/log-dep)
|
We can't actually remove the dependency on |
|
Also, change the signature of all the constructors to return |
What would be the breaking change? The log.Logger interface (currently aliased to core), is present in logger v1. Core hasn't released the Logger interface just yet. So that'd be using LoggerBase / LoggerV2 for everything in the SDK and we should still be able to pass a cosmossdk.io/log/Logger in there I think. We've added a dependency of core in logger to not directly import If we were to release log with cosmossdk.io/core as dependency, it won't work with v0.47 and v0.50 anymore. |
|
Wait a second if the |
|
Okay, I've updated this PR to remove the Constructor functions in |
log/logger.go
Outdated
| type LoggerV2 interface { | ||
| LoggerBase | ||
|
|
||
| // WithContext returns a new wrapped logger with additional context provided by the key value pairs. | ||
| // The returned value can be safely cast to LoggerV2. An any is returned instead of LoggerV2 | ||
| // to avoid the need for log users to import the log package directly. | ||
| WithContext(keyVals ...any) any | ||
| } |
There was a problem hiding this comment.
can we make these internal? it will be confusing to see which one to use
There was a problem hiding this comment.
Meaning put them in an internal package? We can just avoid declaring the interface at all and then do it in log/v2 if we want
github-actions
bot
added
C:x/staking
C:CLI
C:x/gov
C:x/slashing
C:x/distribution
…endency free (#21045) Co-authored-by: marbar3778 <marbar3778@yahoo.com> Co-authored-by: Julien Robert <julien@rbrt.fr> (cherry picked from commit 5c90246) # Conflicts: # runtime/v2/go.mod # schema/module_schema_test.go # server/v2/go.mod # server/v2/go.sum # server/v2/store/server.go # simapp/v2/simdv2/cmd/testnet.go
* main: feat(log): remove core dependency and update core interface to be dependency free (#21045) chore: fix some comments (#21085) feat: simulate nested messages (#20291) chore(network): remove `DefaultConfigWithAppConfigWithQueryGasLimit` (#21055) fix(runtime): remove `appv1alpha1.Config` from runtime (#21042) feat(simapp/v2): Add store server to testnet init cmd (#21076) chore(indexer/postgres): update to changes on main (#21077) feat(schema/appdata): async listener mux'ing (#20879) ci: Use large box for 052 branch sims on CI (#21067) chore(all): replace all `fmt.Errorf` without paramters with `errors.New` (#21068)
Description
In
cosmossdk.io/schemaa logger interface is defined without theWithfunction because this function depends on the concreteLoggertype incore. There is no way to define an expectedLoggerinterface with theWithmethod using the current design.This PR proposes a new
LoggerV2interface with aWithV2method that returns ananythat can be cast toLoggerV2. With this interface, schema or other libraries could use the full logging functionality without a direct dependency oncoreorlog(whichschemaseeks to avoid).Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
New Features
LoggerV2, enhancing flexibility and extensibility in logging functionalities.WithContextmethod to improve context handling in logging.Bug Fixes
Refactor